service.instance.id implementation #6226
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6226 +/- ##
============================================
+ Coverage 91.07% 91.09% +0.01%
- Complexity 5751 5757 +6
============================================
Files 626 627 +1
Lines 16782 16790 +8
Branches 1718 1718
============================================
+ Hits 15285 15295 +10
+ Misses 1003 1002 -1
+ Partials 494 493 -1 ☔ View full report in Codecov by Sentry. |
zeitlinger
force-pushed
the
service-instance-id
branch
from
a50d854 to
9cde2bc
Compare
|
Updated the PR to reflect the spec PR as it was merged. |
zeitlinger
changed the title
...main/java/io/opentelemetry/sdk/autoconfigure/internal/ServiceInstanceIdResourceProvider.java
Outdated
Show resolved
Hide resolved
...main/java/io/opentelemetry/sdk/autoconfigure/internal/ServiceInstanceIdResourceProvider.java
Outdated
Show resolved
Hide resolved
...main/java/io/opentelemetry/sdk/autoconfigure/internal/ServiceInstanceIdResourceProvider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Couple of nits but looks good to me.
@open-telemetry/java-approvers this PR implements a recent change merged to semantic conventions. It has not bee formally released, and the service.instance.id attribute is still marked as experimental. So there's a valid question of whether we should include this attribute by default or not.
If we're nervous about merging this until stable, we could include it in the sdk extension incubator project instead. The inclusion of the incubator on the classpath acting as an "opt in" from the user.
...e/src/main/java/io/opentelemetry/sdk/autoconfigure/internal/EnvironmentResourceProvider.java
Show resolved
Hide resolved
...main/java/io/opentelemetry/sdk/autoconfigure/internal/ServiceInstanceIdResourceProvider.java
Outdated
Show resolved
Hide resolved
|
I agree that a release is desirable before merging this: until it's released, changes are always possible, even though they are unlikely for this specific case. About it being experimental: I believe this change isn't causing backward compatibility issues, other than possibly for cardinality concerns when it comes to metrics. If possible, I'd love this to be opt-out, as it would help us make that semconv stable within a few releases. |
...main/java/io/opentelemetry/sdk/autoconfigure/internal/ServiceInstanceIdResourceProvider.java
Outdated
Show resolved
Hide resolved
…tected using resource provider
zeitlinger
force-pushed
the
service-instance-id
branch
from
35bcc88 to
5668fe3
Compare
Implements service.instance.id spec PR